Skip to content

Assert every ref a transaction writes, not just the first - #4315

Merged
Fokko merged 1 commit into
apache:mainfrom
avison9:fix/transaction-per-ref-requirements
Nov 10, 2026
Merged

Fokko merged 1 commit into
apache:mainfrom
avison9:fix/transaction-per-ref-requirements

Conversation

@avison9

@avison9 avison9 commented Nov 9, 2026

Copy link
Copy Markdown
Contributor

Closes #4183

Rationale for this change

A transaction that writes two refs only guarded the first one against concurrent changes.
Transaction._stage de-duplicated requirements by type alone, so after the first write staged
AssertRefSnapshotId(ref="b"), the second write's AssertRefSnapshotId(ref="main") was dropped and
the commit reached the catalog without an assertion on main.

The type-only de-duplication was correct when it was added (#569), because a transaction could only
ever assert main. Writing to branches (#941) made AssertRefSnapshotId per-ref, but the
de-duplication was never updated.

This changes the key to (type, ref) for AssertRefSnapshotId. Each ref keeps its first (base)
assertion, matching Java's UpdateRequirements. Other requirement types are unchanged. Retries go
through _stage too, so they now keep every ref's assertion as well.

cat.load_table("ns.t").manage_snapshots().rollback_to_snapshot(s1).commit()   # another writer
with t.transaction() as tx:
    tx.append(row(3), branch="b")
    tx.append(row(3))

cat.load_table("ns.t").scan()   # before: [1, 2, 3]  rollback silently undone
                                # after:  [1, 3]     conflict detected, commit retried

Measured on a transaction that writes branch b and then main:

Concurrent change to main Format Before After
rollback v1 / v2 rollback undone retried, [1, 3]
append v1 appended row lost retried, [1, 2, 3, 4]
append v2 non-retryable ValueError (sequence number) retried, [1, 2, 3, 4]

Are these changes tested?

Yes. Two tests were added to tests/table/test_commit_retry.py, and each runs against every catalog
fixture:

  • test_transaction_writing_two_branches_detects_concurrent_change_to_either covers the issue's
    scenario on format v1 and v2, with both a concurrent rollback and a concurrent append.
  • test_transaction_stages_one_ref_assertion_per_ref checks that writing b and then main twice
    stages exactly one assertion per ref, each holding the base snapshot id.

Both fail when the _stage change is reverted. Unit suite: 4524 → 4530 passed, no new failures.
make lint is clean. make test-integration passes (1362 passed, 0 failed).

Are there any user-facing changes?

Yes, as a bug fix. Multi-ref transactions now detect concurrent changes to every ref they write, and
retry or fail with CommitFailedException instead of overwriting them. There is no API change.

Transaction._stage de-duplicated requirements by type, so a transaction
writing two branches only sent AssertRefSnapshotId for the first one and
a concurrent change to the second was overwritten. Key ref assertions by
ref name so each ref keeps its base assertion, as Java does.

Closes apache#4183

@Fokko Fokko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🙌 The per-ref requirement key is the right fix, and the two-branch test that fails on main is exactly the regression this needed. Thanks @avison9!

@Fokko
Fokko added this pull request to the merge queue Nov 10, 2026
Merged via the queue into apache:main with commit 533a617 Nov 10, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A transaction writing two branches overwrites concurrent changes to the second

2 participants